Add a cluster level collector for node config settings #298

rguo-aws · 2020-07-22T18:02:16Z

Issue #:
#300

Description of changes:

Add a cluster level collector for node config settings
create a thread-safe node config cache in AppContect to store node config settings and share them among different RCA vertices in RCA graph

Tests:
tested on docker

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… config cache in AppContext

codecov · 2020-07-22T18:08:09Z

Codecov Report

Merging #298 into master will decrease coverage by 0.13%.
The diff coverage is 67.69%.

@@             Coverage Diff              @@
##             master     #298      +/-   ##
============================================
- Coverage     66.71%   66.58%   -0.14%     
- Complexity     1869     1896      +27     
============================================
  Files           276      284       +8     
  Lines         12307    12559     +252     
  Branches        982     1019      +37     
============================================
+ Hits           8211     8362     +151     
- Misses         3764     3848      +84     
- Partials        332      349      +17

Impacted Files	Coverage Δ	Complexity Δ
...lyzer/rca/store/collector/NodeConfigCollector.java	`80.00% <ø> (ø)`	`6.00 <0.00> (?)`
...ca/store/collector/NodeConfigClusterCollector.java	`48.57% <48.57%> (ø)`	`6.00 <6.00> (?)`
...eanalyzer/rca/store/collector/NodeConfigCache.java	`88.46% <88.46%> (ø)`	`4.00 <4.00> (?)`
.../elasticsearch/performanceanalyzer/AppContext.java	`92.10% <100.00%> (+0.43%)`	`11.00 <1.00> (+1.00)`
...a/framework/api/flow_units/NodeConfigFlowUnit.java	`36.36% <100.00%> (+1.98%)`	`6.00 <1.00> (+1.00)`
...h/performanceanalyzer/rca/framework/core/Node.java	`89.83% <100.00%> (+1.89%)`	`30.00 <1.00> (+2.00)`
...analyzer/threads/exceptions/PAThreadException.java	`0.00% <0.00%> (-57.15%)`	`0.00% <0.00%> (-1.00%)`
...analyzer/rca/framework/core/MetricsDBProvider.java	`21.05% <0.00%> (-21.06%)`	`4.00% <0.00%> (-1.00%)`
...ch/performanceanalyzer/threads/ThreadProvider.java	`64.70% <0.00%> (-17.65%)`	`4.00% <0.00%> (ø%)`
.../performanceanalyzer/rca/framework/api/Metric.java	`76.19% <0.00%> (-14.29%)`	`8.00% <0.00%> (ø%)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbd8675...3363e16. Read the comment docs.

vigyasharma · 2020-07-23T03:00:05Z

...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java

+    if (ret == null) {
+      return Double.NaN;
+    }


Since this is not going to sql db, I don't think we need NaN value and check. Let's instead throw a checked exception here for consumer code safety? (anyway the check has to be handled by the caller; exception will help protect against cases where callers forget the NaN check).

done, throw an exception if config does not exist in cache

...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java

khushbr · 2020-07-23T17:57:38Z

...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java

+  }
+
+  public void put(NodeKey nodeKey, Resource config, double value) {
+    nodeConfigCache.put(new NodeConfigKey(nodeKey, config), value);


Do we want to check here for Non-Null, Non-Negative values before putting them in HashMap?

we perform this check in the node level NodeConfigCollector when collection configs from DB.

...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java

khushbr · 2020-07-23T18:03:58Z

...distro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigClusterCollector.java

+  private final NodeConfigCollector nodeConfigCollector;
+
+  public NodeConfigClusterCollector(final NodeConfigCollector nodeConfigCollector) {
+    super(0, 5);


The constants here make the code unreadable, if we can add a variable with name suggesting what the constant represents, it is helpful.

Additionally, later we should refactor to introduce a defaultIntervalPeriod and use is across all collector and similarly for RCAs as well.

yes, we can create a static const here. But since we will do another refactoring to remove the 5 second intervals form the super() constructor completely, it would be better to leave it as is at this moment ?

Makes sense, please add an issue for this.

...distro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigClusterCollector.java

adityaj1107 · 2020-07-23T18:08:13Z

...amazon/opendistro/elasticsearch/performanceanalyzer/rca/store/collector/NodeConfigCache.java

+  //unbounded cache with eviction timeout set to 10 mins
+  public NodeConfigCache() {
+    nodeConfigCache =
+        CacheBuilder.newBuilder()


Just Curious: Any particular reason we picked the unbounded cache? We may not be having fixed number of entries we want to put in the cache. So wouldn't leaving it unbounded might cause out of memory errors? It should be fine, If we are sure the number of entries are less.

the maxmium number of entries will be (number of node) * (node config on each node) which is not that big considering that stale entries will be evicted if reaches TTL.

adityaj1107 · 2020-07-23T18:09:33Z

...amazon/opendistro/elasticsearch/performanceanalyzer/store/collector/NodeConfigCacheTest.java

+  }
+
+  @Test
+  public void testSetAndGetValue() {


Maybe 1 UT for updating the value of the key and asserting if the updated value is returned.

Add a cluster level collector for node config settings. Create a node…

1816e47

… config cache in AppContext

Add more comments

9de4cd2

rguo-aws requested review from khushbr, sidheart, vigyasharma and adityaj1107 July 22, 2020 18:12

vigyasharma suggested changes Jul 23, 2020

View reviewed changes

adityaj1107 requested a review from vigyasharma July 23, 2020 17:48

khushbr reviewed Jul 23, 2020

View reviewed changes

adityaj1107 reviewed Jul 23, 2020

View reviewed changes

rguo-aws added 3 commits July 23, 2020 15:43

Address PR comments

193f2bb

add javadoc

faa0667

Fix UTs

3363e16

rguo-aws linked an issue Jul 23, 2020 that may be closed by this pull request

Cluster level node config collector #300

Closed

rguo-aws added the feature New feature label Jul 23, 2020

vigyasharma approved these changes Jul 24, 2020

View reviewed changes

khushbr approved these changes Jul 24, 2020

View reviewed changes

rguo-aws merged commit 531b034 into master Jul 24, 2020

rguo-aws deleted the rguo-cluster-node-config branch July 24, 2020 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a cluster level collector for node config settings #298

Add a cluster level collector for node config settings #298

rguo-aws commented Jul 22, 2020 •

edited

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading

vigyasharma Jul 23, 2020

rguo-aws Jul 23, 2020

khushbr Jul 23, 2020

rguo-aws Jul 23, 2020 •

edited

Loading

khushbr Jul 23, 2020

rguo-aws Jul 23, 2020 •

edited

Loading

khushbr Jul 24, 2020

adityaj1107 Jul 23, 2020

rguo-aws Jul 23, 2020

adityaj1107 Jul 23, 2020

Add a cluster level collector for node config settings #298

Add a cluster level collector for node config settings #298

Conversation

rguo-aws commented Jul 22, 2020 • edited Loading

codecov bot commented Jul 22, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rguo-aws Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rguo-aws Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rguo-aws commented Jul 22, 2020 •

edited

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading

rguo-aws Jul 23, 2020 •

edited

Loading

rguo-aws Jul 23, 2020 •

edited

Loading